cc @ronag

@BethGriggs this would need to be pulled in v13 and/or I would remove b03845b from there asap anyway.

CI: https://ci.nodejs.org/job/node-test-pull-request/25714/

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2012/

I'm a big -1 on this.

Instead of reverting the whole thing, please at least let me make a PR that addresses the specific behavioural change referenced in #29699.

Including:

• Documentation for the behaviour.
• A big note in the code.
• Added test for expected compat.
• Workaround for relevant places (if any) in core that uses finished e.g. async iterators.

I can have this done by the end of the day.

Also, I would ask us to continue the discussion in #29699 for finding a long term sustainable strategy for this and similar issues. This indirectly impacts pending work and PR's I have that could end up in a similar situation.

With this I would be -0.

So long as the changes do not go out in a release and @ronag and @mafintosh can collaborate on a revised acceptable solution, then I think we can afford to hold off reverting this immediately. I think we should time box that however, and the change should definitely be pulled out of the 13.0.0 working branch for now while the fix is being worked on.

I believe I have found an elegant solution which fixes #29699 and possibly make everyone happy (fingers crossed). #29724

@mcollina Unfortunately, there's a merge conflict to be resolved.

@jasnell Do you have a proposal for what should be the time-box deadline? I suppose if @BethGriggs has an opinion, that's probably the opinion we should pay the most attention to.

I'd prefer not picking the commit out of the v13.x branch and release - it has already gone into the first RC, and would need to be forced out of a protected branch.

I can pull in this PR or #29724 into the v13.x release once either of those has landed (subject to no TSC objections as per https://github.com/nodejs/node/blob/master/doc/releases.md#release-branch). My plan is to update the v13.x-proposal each Tuesday up until 15th October - after which I'd like to avoid pulling any additional commits into the release.

Then I think we should land this asap and then reapply the PR with the fix if we can land it in time.

I'm happy with whichever approach @BethGriggs and @mcollina are

Am I correct that the two options are either rebase this and land it soon, or we can land #29724? (And in either event, the fix needs to get into the 13.x release.) If we know which one we're going with, we can add it to the 13.x milestone.

Given that we are running out of time, I propose we land this as soon as possible (tomorrow) as a precaution. That commit should not reach v13 as-is and we should likely do a an RC release without it.

Yes, I'm +1 on reverting at this stage as well. This is all super tricky stuff and we need to understand the changes better before trying to land a fix.

Given that we are running out of time, I propose we land this as soon as possible (tomorrow) as a precaution. That commit should not reach v13 as-is and we should likely do a an RC release without it.

Yes, I'm +1 on reverting at this stage as well. This is all super tricky stuff and we need to understand the changes better before trying to land a fix.

Based on this, I've added it to the 13.x milestone.

@mcollina This needs a rebase.

@nodejs/tsc This is labeled semver-major so will need a second TSC review.

Also needs to revert ce62e96 to avoid the conflict.

CI: https://ci.nodejs.org/job/node-test-pull-request/25930/ ✔️

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2043/

This time, without a typo in the ref so it might actually run the tests.

CITGM seems ok to me, landing.

Landed in 7682874...9f873b3

@BethGriggs would you mind adding this to v13?

@mcollina I've pulled it into #29504 and I'm currently building rc-2